Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutation coverage fix within lambdas #1362

Merged
merged 6 commits into from
Nov 11, 2024
Merged

Conversation

see-quick
Copy link
Contributor

This PR fixes the problem described [1].

[1] - #1360

Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
@hcoles
Copy link
Owner

hcoles commented Nov 6, 2024

This looks like it will remove mutations defined in all lambdas, not just lambdas defined within the an annotated method. This is why a test is failing in the overall suite.

I think a minimum set of test cases would be

  • unannotated method
  • annotated method
  • lambda within unannotated method
  • lambda within annotated method
  • lambda within unannotated overloaded method
  • lambda within annotated overloaded method
  • lambda within lambda within unannotated overloaded method
  • lambda within lambda within annotated overloaded method

Signed-off-by: see-quick <maros.orsak159@gmail.com>
@see-quick
Copy link
Contributor Author

This looks like it will remove mutations defined in all lambdas, not just lambdas defined within the an annotated method. This is why a test is failing in the overall suite.

I think a minimum set of test cases would be

  • unannotated method
  • annotated method
  • lambda within unannotated method
  • lambda within annotated method
  • lambda within unannotated overridden method
  • lambda within annotated overridden method
  • lambda within lambda within unannotated overridden method
  • lambda within lambda within annotated overridden method

Yeah I will add those tests. Thanks for pointing it out.

.filter(MethodTree::isGeneratedLambdaMethod)
.filter(lambdaMethod -> {
String lambdaName = lambdaMethod.rawNode().name; // e.g., lambda$fooWithLambdas$0
String enclosingMethodName = extractEnclosingMethodName(lambdaName);
Copy link
Owner

@hcoles hcoles Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here is based solely on method name. If you were to add the other suggested tests they would fail as all lambdas in all the overloads of a method would be filtered if any one of the overloads was annotated.

The method signature must also be taken into account.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the tests. Also, the logic should be adjusted to overridden methods. Let me know if there's something I miss...Thanks

Copy link
Contributor Author

@see-quick see-quick Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm interesting that the build is failing after 1m but locally my build is successful without any problems.

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for pitest-parent dev-SNAPSHOT:
[INFO] 
[INFO] pitest-parent ...................................... SUCCESS [  0.365 s]
[INFO] Build Config ....................................... SUCCESS [  0.295 s]
[INFO] pitest ............................................. SUCCESS [  6.108 s]
[INFO] pitest-entry ....................................... SUCCESS [ 51.068 s]
[INFO] pitest-html-report ................................. SUCCESS [  0.782 s]
[INFO] pitest-aggregator .................................. SUCCESS [  0.795 s]
[INFO] pitest-maven ....................................... SUCCESS [  5.872 s]
[INFO] pitest-command-line ................................ SUCCESS [  0.637 s]
[INFO] pitest-ant ......................................... SUCCESS [  0.626 s]
[INFO] pitest-java8-verification .......................... SUCCESS [  3.993 s]
[INFO] pitest-maven-verification .......................... SUCCESS [02:09 min]
[INFO] pitest-modern-verification ......................... SUCCESS [  0.670 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  03:20 min
[INFO] Finished at: 2024-11-08T10:39:43+01:00
[INFO] ------------------------------------------------------------------------

with mvn clean install
and

Apache Maven 3.9.9 (8e8579a9e76f7d015ee5ec7bfcdc97d260186937)
Maven home: /opt/homebrew/Cellar/maven/3.9.9/libexec
Java version: 17.0.12, vendor: Homebrew, runtime: /opt/homebrew/Cellar/openjdk@17/17.0.12/libexec/openjdk.jdk/Contents/Home
Default locale: en_SK, platform encoding: UTF-8

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think overrides need to be considered, just overloads within the same class. Although it is possible to navigate the class hierarchy from the bytecode, it becomes quite expensive when there are a lot of mutants.

Unhelpfully, I used the word override when I meant overload earlier when describing the test cases.

The failing test is likely because the suite runs a matrix of different java versions. Sometimes the bytecode generated differs substantially with version. Tests need to be careful what they assert on in case they are making implicit assumptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated for overloads. Tested also with my code and it seems to work fine. Build locally is passing now I have to figure it out to fix it also upstream :D

Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
@see-quick
Copy link
Contributor Author

It seems I have fixed everything. Could you look at that when you have some free time @hcoles? Many thanks.

@hcoles hcoles merged commit 35a8ce8 into hcoles:master Nov 11, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants